bundle: record and read deployment state via DMS - #6094
Conversation
Integration test reportCommit: b4d23d2
28 interesting tests: 27 FAIL, 1 KNOWN
|
| // of and the next deploy would create them a second time. Checked here rather | ||
| // than only where operations are recorded, which is after the resource has | ||
| // already been modified. | ||
| if err := opQueue.firstErr(); err != nil { |
There was a problem hiding this comment.
we could eventually extend this to record and return all multiple errors that happened.
| @@ -0,0 +1,4 @@ | |||
|
|
|||
| === An operation upload failure fails the deploy instead of reporting only at the end | |||
There was a problem hiding this comment.
it's hard to make a assert more here because we cannot control how many requests went through. We could harden this test by making the number of workers configurable and 1. Omitting for now.
| @@ -0,0 +1,18 @@ | |||
| Local = true | |||
| Cloud = false | |||
There was a problem hiding this comment.
We'll rollout the service to our test env by next week at which point we can start turning these tests on on cloud as well.
|
|
||
| # Deployment Metadata Service (DMS) recording is only supported by the direct | ||
| # engine; it is a no-op on terraform. | ||
| EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] |
There was a problem hiding this comment.
Eventually we can also make DMS part of the default test matrix - to get more coverage - this can be rolled out slowly.
| assert.Empty(t, f.recorded()) | ||
| } | ||
|
|
||
| func TestOperationQueueCloseIsIdempotent(t *testing.T) { |
There was a problem hiding this comment.
this is not strictly necessary given we only have one close site.
Approval status: pending
|
f38cb18 to
a49f16f
Compare
| return &apiVersionCreator{client: c} | ||
| } | ||
|
|
||
| func (a *apiVersionCreator) CreateVersion(ctx context.Context, deploymentID, versionID string, body createVersionRequest) (*bundledeployments.Version, error) { |
There was a problem hiding this comment.
will be replaced with SDK calls soon in a followup.
| // The extra field cannot be added by embedding Deployment in a wrapper struct: | ||
| // Deployment has its own MarshalJSON, which is promoted to the wrapper and | ||
| // silently drops any sibling field. | ||
| func deploymentBody(d *dmsDeployment) (map[string]any, error) { |
There was a problem hiding this comment.
goes away on a SDK bump
| r := NewRecorder(RecorderOptions{Service: f, Versions: fakeVersions{requests: &f.versions}, DeploymentID: "stored-id", StatePath: testStatePath, Metadata: Metadata{TargetName: "dev", DisplayName: testDisplayName}, VersionType: VersionTypeDeploy}) | ||
|
|
||
| err := r.CreateVersion(t.Context()) | ||
| assert.ErrorContains(t, err, "internal error: no deployment found for the file with object id stored-id") |
There was a problem hiding this comment.
this should never happen if the service is working properly, since htis indicates that the file exists but there was no deployment returned by GET deployment.
| // (sv.Value) come from the write just performed; GetResourceID reads | ||
| // the ID assigned by Deploy. depends_on is recorded alongside the config | ||
| // because it cannot be recomputed from it (see dstate.RecordedState). | ||
| if err := opQueue.record(ctx, resourceKey, action, b.StateDB.GetResourceID(resourceKey), sv.Value, d.DependsOn); err != nil { |
There was a problem hiding this comment.
Correct me if I'm wrong, this records "recreate" once, correct?
We discussed sending two requests - one for Delete and one for subsequent Create.
We also discussed moving recording hooks inside SaveState, so that we record the event ASAP. I'm considering for some resources to record state earlier and/or multiple times, by moving recording into SaveState we'll be able to handle that with DMS #5391
| "github.com/databricks/databricks-sdk-go/service/bundledeployments" | ||
| ) | ||
|
|
||
| // The CLI calls the operations API directly rather than through the generated |
There was a problem hiding this comment.
We need to fix this. I'll investigate in a followup.
|
|
||
| // The server expires a version's lease if it does not receive a heartbeat | ||
| // within a 2-minute TTL; we heartbeat well inside that window. | ||
| const defaultHeartbeatInterval = 30 * time.Second |
There was a problem hiding this comment.
Parts of the code here should be removed and replaced by the SDK soon.
| // node under the state directory, so files.Delete removes it and any later call | ||
| // fails with 404. CompleteVersion is idempotent, so the deferred call in Destroy | ||
| // is a no-op after this. | ||
| if err := recorder.CompleteVersion(ctx, true); err != nil { |
There was a problem hiding this comment.
Its a bit of a chicken and egg problem. We can improve this by first cleaning up worksapce.file_path + artifact_path. Then completing the version and then deleting state_path + root_path.
Left as a followup exercise, the current PR is already big enough in scope.
| # unset; `deploy --plan` then creates the resources without it and the next plan reports | ||
| # drift. Stamping at plan time would mean `bundle plan` creating the deployment record, | ||
| # which is a design decision, so the saved-plan path is left out of the DMS run for now. | ||
| EnvMatrixExclude.dms_no_readplan = ["DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY=true", "READPLAN=1"] |
There was a problem hiding this comment.
We'll fix this in a followup.
| // failureFields. A failure that arrives before any operation exists still | ||
| // goes through CreateOperation below, carrying the prior state. | ||
| fields = failureFields | ||
| body = updateOperationRequest{ |
There was a problem hiding this comment.
By omitting state here we ensure that the latest state that was uploaded in a partial operation is retained.
…lates (databricks#6220) ## Changes `bundle init` now writes `experimental.record_deployment_history: true` into the generated `databricks.yml` when `DATABRICKS_BUNDLE_INIT_RECORD_DEPLOYMENT_HISTORY` is set. It covers every built-in template that generates a `databricks.yml`, and is off by default so template output is otherwise unchanged. ## Why Lets new projects opt into recording deployment history at creation time instead of editing the config by hand afterwards. The variable is temporary and deliberately scoped to `bundle init`: it persists the setting in the generated project rather than toggling it for the bundle being run. Note the setting is still gated off (databricks#6094), so a generated project also needs `DATABRICKS_BUNDLE_FORCE_ALLOW_RECORD_DEPLOYMENT_HISTORY` to validate until that gate is lifted. The acceptance test sets both and runs `bundle validate`, so it will fail if the two ever drift apart. ## Tests New acceptance test: all five templates with the variable on, plus the default-off and explicitly-false cases.
| Service bundledeployments.BundleDeploymentsInterface | ||
|
|
||
| // raw sends what the generated client cannot; see requester. | ||
| raw requester |
There was a problem hiding this comment.
this will go away once we get everything in the SDK
There was a problem hiding this comment.
general comment: please add such comments directly to code, otherwise they will be lost.
There was a problem hiding this comment.
q: What's still missing from SDK?
There was a problem hiding this comment.
q: What's still missing from SDK?
A couple of that need to be propogated to the SDK from the API spec. Some are still marked as development and need to be marked as PrPr.
There was a problem hiding this comment.
let's mark it as PrPr and use generated client?
47e54da to
766a902
Compare
Record each deploy's resource state to the deployment metadata service and read it back, making the service the source of truth for a recorded bundle: the local state file becomes a tombstone and a leftover WAL is discarded. Gated on experimental.record_deployment_history (direct engine only), and supports deploy --plan. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…anged Co-authored-by: Isaac <no-reply@databricks.com>
…source Co-authored-by: Isaac <no-reply@databricks.com>
…, drop dedicated summary test Co-authored-by: Isaac <no-reply@databricks.com>
…op redundant operation-upload-message test Co-authored-by: Isaac <no-reply@databricks.com>
…, drop serialized-plan
…rces.get_recorded_state
… stub instead of fault.py
766a902 to
1b0b45f
Compare
…pen reads the workspace client from context Co-authored-by: Isaac <no-reply@databricks.com>
…hem from DeploymentBundle The phases now reach DMS through StateDB (DmsClient, OperationBuffer, TakeOperationBuffer) rather than DeploymentBundle mirror fields. The operation drain stays before CompleteVersion. Co-authored-by: Isaac <no-reply@databricks.com>
…ffer in StateDB - add dstate.StorageBackend (WORKSPACE_FILESYSTEM/DEPLOYMENT_METADATA_SERVICE), set by Open from the state marker; the plan carries its string form so deploy --plan can reject a plan whose target switched backends - StateDB owns the recording lifecycle: RecordingError() and CompleteVersion() (idempotent via versionCompleted), with the operation-buffer drain moved into Finalize; drop the exposed OperationBuffer/TakeOperationBuffer getters and the recorder() indirection - gate DMS behavior on StorageBackend()==DEPLOYMENT_METADATA_SERVICE / ConfiguresDeploymentHistory instead of DmsClient()!=nil - consolidate plan validation in validatePlan (config-vs-plan backend match + stale-plan guard) Co-authored-by: Isaac <no-reply@databricks.com>
…ate unbind, restore v3 read - rename StampDeploymentID -> StampDeploymentIdForFirstVersion (only used for the first version's id) - inline RecordDeploymentHistoryEnv into RecordsDeploymentHistory (single predicate) - unbind: drop the config-based phase check; DeploymentBundle.Unbind opens without recording, so the state marker (not the config flag) refuses a recorded deployment - restore forward-compat read of featureStateVersion=3: this CLI still writes v2, but reads a v3 state with no features and refuses one that carries a feature Co-authored-by: Isaac <no-reply@databricks.com>
Extend declined-deploy, depends-on and deployment-metadata-change to capture the serialized 'bundle plan -o json' (now carrying storage_backend), the DMS list-resources view, and the raw recorded state. deployment-metadata-change pins state_path, which print_state.py cannot resolve, so it relies on list-resources. Co-authored-by: Isaac <no-reply@databricks.com>
…ate across the suite Extend no-resources, multiple-resources, record, successful-recreate, no-drift, stale-plan, emptied-resource and provenance to capture the serialized 'bundle plan -o json' (now carrying storage_backend), the DMS list-resources view, and the raw recorded state at each steady-state point. no-drift shows only the updated resource bumping to version 2; successful-recreate shows the recreate at version 2; provenance masks the git commit. Co-authored-by: Isaac <no-reply@databricks.com>
…-version golden
- deploy.go: StorageBackend() asserts the state is open, but only the direct engine
opens it, so gate the DMS deployment-create block on stateEngine.IsDirect() first.
Fixes a panic ("DeploymentState must be opened first") on terraform deploys, which
the direct-only dms tests did not exercise.
- future_version: supported state version is now 3 (v3 is read-forward-compatible),
so the too-new message reports 3.
Co-authored-by: Isaac <no-reply@databricks.com>
…version on recording error - C11: StateDB holds DeploymentID/LatestVersionID (set at Open from the service); CalculatePlan stamps the plan's lineage from StateDB instead of the bundle config tree (config.History stays only for 'bundle summary'). Behavior-neutral. - unbind: Open returns a sentinel ErrUnsettingRecording; DeploymentBundle.Unbind maps it to an unbind-specific message instead of the deploy/destroy-flavored guard text. - CompleteVersion now completes as Failure when the drained buffer holds a recording error, even if the caller passed success=true, so a destroy whose uploads failed keeps its deployment record. Co-authored-by: Isaac <no-reply@databricks.com>
… the readplan option The dedicated readplan test duplicated stale-plan, which already deploys from a saved raw plan under recording. Delete it and add the standard READPLAN matrix to stale-plan so its initial deploy runs both from the saved plan and recomputed - the deploy is untraced so the variant-dependent command stays out of the golden. Recorded operations still carry the deployment_id/version_id lineage (asserted in depends-on and others). Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Extend the failure-injection tests to show how DMS records a failed or refused deploy: failed-delete/recreate/update, failed-update-permissions, operation-upload-fails-once, record-failure, version-never-created (and a WORKSPACE_FILESYSTEM plan on existing-state's first deploy). Each captures the healthy recorded state and the recorded result of the failure. Co-authored-by: Isaac <no-reply@databricks.com>
… wording - rename StartRecording -> InitializeOperationBuffer (it opens the buffer; kept separate from Open because a first deploy's id does not exist until after approval) - collapse the duplicated deployment/version fields to one public pair (DeploymentID/LatestVersionID); InitializeOperationBuffer records the first-deploy id, CompleteVersion derives the version from it - RecordingError is now a no-op unless the backend is the deployment metadata service, keeping the non-recording path cleanly separate (like RecordFailure) - stop surfacing the internal "deployment metadata service" name in user-facing errors; say "deployment history feature enabled" / "the deployment history service" - drop the "in place of nil client checks" comment and restore the original featureStateVersion wording (minimal C12 diff) Co-authored-by: Isaac <no-reply@databricks.com>
Summary
This PR adds code to read and write state using DMS, behind
DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY.Design decisions:
CreateVersionstages one operation per planned resource, so the CLI only ever callsUpdateOperation— there is noCreateOperationcall. The service creates each staged operation asPENDINGatsequence_id = 0, which is the precondition the CLI uses for its first update of a resource. Needs databricks-eng/universe#2420238 (merged).Also adds an env var to toggle DMS, and records the API status and error code with a failure.
Testing Strategy
The whole acceptance suite runs a second time with recording on (
EnvMatrix.DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY), so every bundle test exercises DMS against the local test server and asserts the same golden files either way — thenostamphelper strips the deployment stamp for that. Focused coverage of the recorded calls themselves lives underacceptance/bundle/dms.Before private preview we will:
What is missing?